feat(gateway): safely rotate ACME credentials - #1004
Closed
kvinwang wants to merge 3 commits into
Closed
Conversation
Collaborator
Author
|
Superseded by the restored and unstacked #935. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an admin-controlled mechanism in dstack-gateway to rotate the shared ACME account credentials while keeping gateway startup and current TLS serving resilient to corrupt persisted ACME credential JSON.
Changes:
- Add an admin-only
RotateAcmeCredentialsRPC that creates a new ACME account, updates per-domain CAA records, then publishes the replacement credentials to WaveKV. - Make ACME credential JSON parsing “fail closed” (invalid JSON now surfaces as an error for the affected ACME operation, rather than silently falling back).
- Refactor DNS-credential access to support using each ZT domain’s configured DNS credential during rotation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dstack/gateway/src/main_service.rs | Exposes a proxy method that delegates ACME credential rotation to the certbot manager. |
| dstack/gateway/src/distributed_certbot.rs | Implements rotation flow, adds stricter ACME credential JSON validation, and introduces DNS client helper logic. |
| dstack/gateway/src/admin_service.rs | Wires the new admin RPC handler returning account URI + updated domain count. |
| dstack/gateway/rpc/proto/gateway_rpc.proto | Adds RotateAcmeCredentialsResponse and the RotateAcmeCredentials RPC definition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+118
to
+121
| client | ||
| .set_caa_records(&[format!("*.{}", config.domain)]) | ||
| .await | ||
| .with_context(|| format!("failed to update CAA for {}", config.domain))?; |
Comment on lines
447
to
+451
| // Get DNS credential (from config or default) | ||
| let dns_cred = if let Some(ref cred_id) = config.dns_cred_id { | ||
| self.kv_store | ||
| .get_dns_credential(cred_id) | ||
| .context("specified DNS credential not found")? | ||
| } else { | ||
| self.kv_store | ||
| .get_default_dns_credential() | ||
| .context("no default DNS credential configured")? | ||
| }; | ||
| let dns_cred = dns_credential_for(&self.kv_store, config)?; | ||
|
|
||
| // Create DNS client based on provider | ||
| let dns01_client = match &dns_cred.provider { | ||
| DnsProvider::Cloudflare { api_token, api_url } => { | ||
| Dns01Client::new_cloudflare(domain.to_string(), api_token.clone(), api_url.clone()) | ||
| .await? | ||
| } | ||
| }; | ||
| let dns01_client = self.dns_client(domain, config).await?; |
Comment on lines
+77
to
+80
| pub async fn rotate_acme_credentials(&self) -> Result<(String, usize)> { | ||
| let Ok(_guard) = self.caa_lock.try_lock() else { | ||
| bail!("ACME credential rotation or CAA reconciliation is already in progress"); | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A corrupt persisted ACME credential must not take down Gateway or discard certificates that are already serving traffic. Operators also need a controlled way to replace the shared ACME account and its account-bound CAA records.
Solution
RotateAcmeCredentialsRPC.issueandissuewildCAA records for every configured ZT domain, using each domain's configured DNS credential.Gateway does not retain a long-lived ACME client. Each issuance or renewal reloads credentials from WaveKV, so nodes automatically use the replacement credential on their next attempt after synchronization.
Failure isolation
CAA/account creation failures abort rotation without replacing the persisted credential. Existing certificates remain loaded and continue serving traffic. Corrupt credentials fail only the affected ACME operation and can be repaired through the rotation API.
Verification
cargo check -p dstack-gatewaycargo test -p dstack-gateway credential_tests -- --nocapturegit diff --check